Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Allow to append logs after a post processing hook #1025

Merged
merged 3 commits into from
Apr 5, 2022
Merged

Allow to append logs after a post processing hook #1025

merged 3 commits into from
Apr 5, 2022

Conversation

thomas-nguy
Copy link
Contributor

@thomas-nguy thomas-nguy commented Apr 1, 2022 •
edited
Loading

Closes: #1024

Description

Add the ability to append log to the receipt after a tx post processing hook

more information on the issue ticket.

open for discussion

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Apr 1, 2022 •
edited
Loading

Codecov Report

Merging #1025 (8d751c6) into main (8202a09) will decrease coverage by 0.00%.
The diff coverage is 82.14%.

❗ Current head 8d751c6 differs from pull request most recent head 711ae62. Consider uploading reports for the commit 711ae62 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
- Coverage   59.16%   59.15%   -0.01%     
==========================================
  Files          80       80              
  Lines        6577     6576       -1     
==========================================
- Hits         3891     3890       -1     
  Misses       2467     2467              
  Partials      219      219
Impacted Files Coverage Δ
x/evm/keeper/state_transition.go 75.54% <82.14%> (-0.08%) ⬇️

fedekunze
fedekunze suggested changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just modify the logs from the receipt?

@thomas-nguy
Copy link
Contributor Author

Yes, but the computation of the bloom needs to be moved down nevertheless.

Do you prefer to have this ability implicitly and keep the previous signature?

@thomas-nguy thomas-nguy force-pushed the thomas/add-log-to-hook branch from e4ba2bd to 0b02641 Compare April 4, 2022 10:28
@fedekunze
Copy link
Contributor

Yes, but the computation of the bloom needs to be moved down nevertheless.

Why? if you are updating the logs then you should also be updating the bloom during the post tx processing step. We'd need to change the state transition to do the following instead:

	if len(receipt.Logs) > 0 {
		// Update transient block bloom filter
		k.SetBlockBloomTransient(ctx, receipt.Bloom)
		k.SetLogSizeTransient(ctx, uint64(txConfig.LogIndex)+uint64(len(receipt.Logs)))
	}

@thomas-nguy thomas-nguy force-pushed the thomas/add-log-to-hook branch from 120b4c5 to 1cf2f36 Compare April 5, 2022 03:09
@thomas-nguy thomas-nguy changed the title Add the ability to append additional log in the receipt after a post processing hook Allow to append logs after a post processing hook Apr 5, 2022
@thomas-nguy
Copy link
Contributor Author

okay, make sense. Published a minimal change version

@thomas-nguy
add log after tx post processing hook
f04c02a 
add changelog

fix tests

rename variable

minimum change version
@thomas-nguy thomas-nguy force-pushed the thomas/add-log-to-hook branch from 1cf2f36 to f04c02a Compare April 5, 2022 03:22
fedekunze
fedekunze approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
@fedekunze fedekunze enabled auto-merge (squash) April 5, 2022 15:33
@fedekunze fedekunze requested a review from crypto-facs April 5, 2022 15:33
@fedekunze fedekunze merged commit 56c4a31 into evmos:main Apr 5, 2022
@thomas-nguy thomas-nguy mentioned this pull request May 19, 2022
Merged
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fedekunze fedekunze fedekunze approved these changes

@khoslaventures khoslaventures Awaiting requested review from khoslaventures

@jolube jolube Awaiting requested review from jolube

@crypto-facs crypto-facs Awaiting requested review from crypto-facs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the ability to append additional log in the receipt after a post processing hook
2 participants
@thomas-nguy @fedekunze